Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: expose detectors and separate detect and collect functions #144

Merged
merged 15 commits into from
Nov 5, 2023
Merged

feat: expose detectors and separate detect and collect functions #144

merged 15 commits into from
Nov 5, 2023

Conversation

Le0Developer
Copy link
Contributor

See #143 for motivation.

Not sure if getSources() and getDetectors() should be kept, because they are no longer used.

see the linked issue/pr for more information on motivation
@xnerhu
Copy link
Contributor

xnerhu commented Sep 10, 2023

LGTM, but it exposes detectors publicly, so we would be obligated to keep up with backward compatibility. @r-valitov @Finesse wdyt

@Finesse
Copy link
Member

Finesse commented Sep 11, 2023

sending the components to the server and then running detect() there

@Le0Developer Did you really try to run detect on server? As I can see, many detectors use browser APIs, which aren't available in Node.js environment. What you want to achieve requires a deeper refactoring, to make sure the detectors are pure functions (i.e. use no data other that the input arguments, and use no browser API).

Also, could you please clarify why you export detectors? It would make sense if the exported detect function accepted detectors as an argument. Such way you could detectors as a base for your custom list of detectors.

but it exposes detectors publicly, so we would be obligated to keep up with backward compatibility

@xnerhu Not necessarily. You may state in the docs that the detector list type is Record<string, (components: ComponentDict) => DetectorResponse>, and the actual list of functions is outside of version control.

@Le0Developer
Copy link
Contributor Author

Le0Developer commented Sep 11, 2023

As I can see, many detectors use browser APIs, which aren't available in Node.js environment.

IIRC those mostly comprise of functions related to getting the browser kind etc, which shouldn't be hard to move.

What you want to achieve requires a deeper refactoring

Yes, which I planned to do in a later PR, so it's easier to review.

Such way you could detectors as a base for your custom list of detectors

This would require turning the components and detectors dicts into AbtractComponentDict and AbstractDetectorDict respectively, which might break typing? Will need to check. Looks it won't.

@Le0Developer
Copy link
Contributor Author

but it exposes detectors publicly, so we would be obligated to keep up with backward compatibility

@xnerhu That whole export block is already documented as being out of semantic versioning:

BotD/src/index.ts

Lines 49 to 50 in a153850

// The exports below are for private usage. They may change unexpectedly. Use them at your own risk.
/** Not documented, out of Semantic Versioning, usage is at your own risk */

src/sources/browser.ts Outdated Show resolved Hide resolved
src/sources/browser.test.ts Outdated Show resolved Hide resolved
Copy link
Member

@Finesse Finesse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you want to achieve requires a deeper refactoring

Yes, which I planned to do in a later PR, so it's easier to review.

This may contradict an architectural decision made by @r-valitov and @xnerhu. The idea is to separate information sources from simple utility functions. I personally think the code state in the PR is good.

src/sources/browser.ts Outdated Show resolved Hide resolved
src/detector.ts Outdated Show resolved Hide resolved
src/api.ts Outdated Show resolved Hide resolved
@Finesse Finesse self-requested a review September 11, 2023 12:37
Copy link
Member

@Finesse Finesse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I can say it's a high-quality PR, thank you.

The project maintainers will make the final decision about the architecture changes in this PR.

src/sources/android.ts Outdated Show resolved Hide resolved
@Finesse Finesse self-requested a review September 11, 2023 12:44
@xnerhu xnerhu self-requested a review September 13, 2023 11:32
(length === 37 && !arrayIncludes([BrowserEngineKind.Webkit, BrowserEngineKind.Gecko], browserEngine)) ||
(length === 39 && !arrayIncludes([BrowserKind.IE], browser)) ||
(length === 33 && !arrayIncludes([BrowserEngineKind.Chromium], browserEngine))
(length === 37 && !arrayIncludes([BrowserEngineKind.Webkit, BrowserEngineKind.Gecko], browserEngineKind.value)) ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, remove .value from enums.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also @Le0Developer please, merge new changes from main

@xnerhu
Copy link
Contributor

xnerhu commented Nov 5, 2023

@Le0Developer LGTM, thank you.

@xnerhu xnerhu changed the base branch from main to dev November 5, 2023 12:36
@xnerhu xnerhu merged commit bfc36e8 into fingerprintjs:dev Nov 5, 2023
@Le0Developer Le0Developer deleted the feat/better-api branch November 5, 2023 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants